Feature/action - #17
Merged
Merged
Conversation
1 task
added 29 commits
February 18, 2026 17:30
… for timeout detection
… to the async await support for feedback: the feedback has to remain a callback, because otherwise we would need to buffer feedbacks received between the goal acceptance and the implicit feedback callback registration done by co_await feedback(). This buffering would be required to avoid a race.
…re it throws an exception if the action is already done.
…, because the async_goal_result API does not make sense. For this, we move the result promise inside the goalhandle, this leads to the goal handle not being copyable, so we return a shared pointer to the goal handle (like the original ROS API). I've also added three other useful methods from the GoalHandle base
…always launches an asynchronous operation, which in the case of actions API does not hold anymore
… the first action test passes
added 26 commits
February 18, 2026 17:37
…ation functions for all service calls.
…nnecessarily during user callbacks
…rsion of the same type
… adress as the timeout timer id. Reverted back to always suspend in the promise (await ready returns false), because this rules out stack overflows and is required anymore for the actions api. Removed some service apis because they were not implemented correctly. I can still add a two-callback version by adding then and except to the promise, and a cancel method to the promise. Also fixed some merge-related errors, and made result type to work with non-default-constructible types.
iv461
force-pushed
the
feature/action
branch
from
February 18, 2026 22:33
b45f74e to
afc7400
Compare
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements an async await API for both action server and action clients.
It does so by adding a copy of the rclcpp_actions source code and patching some limitations as well as critical bugs that are absolutely necessary for implementing an async await API.
Feature set
Action server
The main limitation in
rclcppwas a missing support for deferred response. So I've split the internal methods handling the service calls in half and moved the second half into separatesend_resposemethods (similar to the ones present in the services API).Patch, see file
icey/include/icey/action/server.hpp/cppAction client
There were a couple of bugs in
rclcppthat prevented a correct implementation:Deadlock occurs when chaining
send_goalrequests. This is caused by non-reentrant mutexes being held locked unnecessarily during user callbacks. This was fixed in the rolling version (async_send_goal deadlocks ros2/rclcpp#2796), but Tomoja Fujita says it won't be backported (async_send_goal deadlocks ros2/rclcpp#2796 (comment)). Unfortunately, this conceptual issue is present in many places inrclcpp. I fixed it by not locking at all during user callbacks (Patch](ef48fc1...ef1258f), file /icey/src/actions/client.cpp, tested by test caseActionTimeoutAndMultipleGoalsTest.)No cancellation API for cancelling
send_goalandcancelrequests (similar to regular service calls) is present. Currently, it is not possible to cancel callbacks or delete a pending goal request. If the server dies, the client leaks memory. This issue has not yet been resolved in the rolling version, as there is currently no way to delete items from thepimpl_->pending_goal_responsesmap added here. I've fixed this by adding returning the request ID of the underlying service calls and then adding cancellation function to cancel these requests (See patch, file icey/include/icey/action/client.hpp)
The action client API problem
One problem that remains unsolved in this PR is the action client API.
The problem is that the action protocol specification as given by the sequence diagrams in the action design document is not implemented correctly by ROS.
Whether or not it is implemented correctly however critically influences the client API.
If we would implement an API that follows the specification, we would get lost feedback messages.
Details
The issue is that per specification, the client should be able to receive feedback messages only after requesting the result request. However, the current ROS implementation does not adhere to this specification. This standpoint is also shared by Janosch Machowinski (ros2/rclcpp#2782 (comment)).
It instead allows the client to receive feedback messages before the result request has been sent.
This means a sensible async/await API such as:
would have a race condition, although it adheres to the specification. Feedback messages may be received before the
get_resultfunction is called, meaning they may be lost. This issue has been discussed here and a fix proposed. The proposed fix lead to the implementation following the specification and therefore enabling the above async/await API.I've decided to provide an API that uses a feedback callback to avoid loosing feedback messages. Once the bug in the underlying actions implementation is fixed, we could switch to the above API.
Other minor changes:
count_subscribersand similar tests